Tunnel-mode reception, and five object-handling fixes (compression, partitioning, transfer length) - #68
Conversation
I thought you weren't intending to release your RaptorQ implementation just yet, @jordijoangimenez. |
|
Yes, it should go into a separate branch as previously intended but this PR 68 carried over too much, so I'll have to dissect. We need to decide how we want to tackle the non-3gpp but still flute-related features |
|
@jordijoangimenez: PR #56 and PR #60 are merged now, so this PR needs to be rebased. |
f4100f9 to
fe9b80e
Compare
fe9b80e to
7190377
Compare
637cf46 to
12fb037
Compare
52f629a to
ec9c1cf
Compare
c7306fc to
e6bbe48
Compare
434e835 to
75f1f72
Compare
4c3e556 to
e6bbe48
Compare
fa2054c to
931d9eb
Compare
45f4df6 to
e90c13d
Compare
edc6a0b to
1d6b22b
Compare
The Receiver-side counterpart to Transmitter's existing udp_tunnel_address() support. Per the issue discussion, the library has no business knowing about any particular encapsulation format -- that is entirely the controlling application's concern: "Stripping the GTP-U header doesn't feel like something that a generic FLUTE library should be asked to do... A better design pattern would be for the controlling application to pass in a 'helper' function that the library invokes to do application-specific mangling of packets before the generic code in the library starts processing the ALC/LCT payload." -- rjb1000 Adds three new optional constructor parameters: - tunnel_address: if given, ALSO bind a plain unicast UDP socket to this local endpoint and accept tunnelled datagrams there, in addition to the normal multicast join. The two paths are independent and both feed the same session state -- deliberately not an either/or choice like Transmitter's tunnel mode, since a Receiver has no way to know in advance which path will actually work in a given deployment. - tunnel_source: if given, only accept tunnel datagrams from this source address -- the tunnel-socket equivalent of the existing source_address parameter's SSM admit-only-this-source semantics. This is the "extra address checking" the library itself does, on top of whatever packet_modifier does; source-address admission is a generic, encapsulation-agnostic concept the library can reasonably own, unlike parsing any particular header format. - packet_modifier: required whenever tunnel_address is set. Given the whole received datagram (as a mutable vector, so a modifier can also decrypt/rewrite in place, not just locate the payload), returns the byte offset at which the ALC/LCT payload begins -- an offset >= the buffer's size means "discard, nothing usable here". No default implementation is provided, since any default would itself bake an encapsulation assumption into the library. This mirrors the de-tunnelling logic that already exists, hand-written, in tests/test_end_to_end.cpp's run_tunnel_bridge() (added alongside Transmitter's own tunnel mode in #56): a std::thread there receives on a plain UDP socket, manually parses a hand-built inner IPv4+UDP header out of the payload, and forwards just the FLUTE bytes onward over loopback to a receiver with no tunnel-awareness at all. This moves that capability inside Receiver proper as a caller-supplied, protocol-agnostic hook, so any deployment's actual encapsulation (Transmitter's own wrapper, real GTP-U, or anything else) is expressed purely by what modifier is passed in -- enabling rt-mbs-client (referenced in the issue as "MBSTF Client") to take advantage of it for reception paths where local multicast delivery isn't available at all. Confirmed live end-to-end: real Service Announcement content broadcast over an actual gNB/UE radio link, captured on the UE's TUN device via a tunnel_address + packet_modifier pairing, correctly parsed into a complete FDT and announcement bundle.
handle_receive_from() and handle_tunnel_receive_from() only called arm_receive()/arm_tunnel_receive() again in the success branch -- a single transient socket error (e.g. an ICMP port-unreachable surfacing as a UDP socket error on a subsequent read, hit live via the raw capture relay's loopback sendto() path) permanently killed reception for the rest of the process's life, with just one log line and no way to recover short of restarting the client. (Attempted as a cross-repo cherry-pick of jordijoangimenez/rt-libflute commit d453c05, 'Fix FDT/TOI reassembly corruption, multicast bind/join, ...' -- turned out this branch already independently has every other fix from that commit (FDT-instance-discard reassembly logic, catch(const char*), the INADDR_ANY bind, the per-interface multicast join, the array-specialised shared_ptr scratch buffers); the re-arm bug above was the only genuinely missing piece, found here by live strace/tcpdump debugging, not by that commit.)
…f hanging Problem calculate_partitioning() divides the transfer length by encoding_symbol_length, then the symbol count by max_source_block_length. Both come straight from the FEC OTI and neither was checked. A default-constructed FecOti leaves both 0, so the first division yields inf, the block count yields inf, and block creation becomes effectively unbounded: the File constructor hangs rather than reporting anything. Reachable by a caller, not only internally: the public File constructors accept a FecOti and never inspect it. Observed as a ctest timeout, not a crash, which is the harder failure to diagnose from a log. [code-derived, no spec claim] Basis No clause governs this; it is an unchecked divisor. Fixable under RULES.md rule 2 with no specification claim. On refusing rather than substituting a default: rule 12 prefers failing loudly, and any value invented here would be a bound resting on nothing. Raised by Writing a test for a different finding on this branch. The test constructed a File from a FileDescription without configuring its FEC OTI, and hung instead of failing, which is how this surfaced. Change Reject a FEC OTI whose encoding_symbol_length or max_source_block_length is zero, before either is used as a divisor, naming both fields in the message. Verification T1: builds clean, ctest at the build root 44/44, up from 40. Four cases added, using the direct File constructor, which is the public path that accepts an unchecked FecOti. The three refusal cases were each confirmed to TIME OUT with the guard removed and to throw with it restored, so they reproduce the hang rather than describe it. The fourth uses a usable FEC OTI and passes either way, holding the guard to refusing only what it should. Not in this change No default substituted for either field, and no validation of the other FEC OTI members. The Raptor partitioning path has its own divisors and is not touched here.
…hout an encoding Problem The File element parser fell back to Content-Length whenever Transfer-Length was absent, without regard to whether a content encoding had been applied. For an encoded object the two are different quantities, so the fallback handed the decompressor the decoded length as its input size, wrong by however much the encoding changed, and decoding failed on a well-formed session. Observed at src/FileDeliveryTable.cpp, File element parse. [code-derived] Basis RFC 3926 clause 3.4.2: "If the file is not content encoded before transport (and thus the "Content-Encoding" attribute is not used) then the transfer length is the length of the original file, and in this case the "Content-Length" is also the transfer length." The substitution is authorised for that case and no other. Raised by reading the parser while tracing why a gzip object failed to decode Change Content-Encoding is now parsed before the transfer length, and the fallback applies only when no encoding was declared. With an encoding applied and no Transfer-Length carried, the transfer length is left at 0, meaning not known from this FDT, rather than guessed. Verification T0: builds clean and the suite passes. The test that pins this rule in both directions arrives with the change that makes the unknown case decodable, since on this commit alone an encoded object has no other source for its length. Not in this change Supplying the length by another route. Withholding a wrong value stops the silent mis-decode; it does not by itself make an encoded object decodable under the MBMS Download Profile, which needs the object's own EXT_FTI.
…har* Problem On a compression failure File::encode() formatted zs.msg through spdlog and then threw it. zlib leaves zs.msg NULL for Z_STREAM_ERROR, so the log call received a null char* (spdlog reports "string pointer is null") and the throw sent a raw char* that only catch(const char*) could take, whose handler would then dereference null. The zlib stream and, where owned, the decompressed buffer were both leaked on the way out. Observed: a transmitter aborting with "terminate called after throwing an instance of 'char*'". Code-derived, no spec claim. Raised by reading the error branch while working on the compression path Change Substitutes a fixed string when zlib supplies no message, logs the zlib status code alongside it, releases the stream and any owned buffer, and throws std::runtime_error so an ordinary catch(std::exception&) handles it. Verification T0: builds clean, and the surrounding suite passes. The branch this lands on has no test able to force a zlib stream error, which would need a fault injection point inside encode() that does not exist. Not in this change The deflate call the loop was missing, which reached development separately.
Problem File::encode() passed windowBits 15|16 to deflateInit2 unconditionally, which is gzip framing, while File::decode() passes 15|((encoding == "gzip") ? 16 : 0) to inflateInit2 and so selects on the declared encoding. A file declared with Content-Encoding "deflate" was therefore written with gzip framing and could not be read back by this same library, and the framing on the wire did not match what the declared encoding means. Observed: src/File.cpp, encode() and decode() disagreeing about the same file. [code-derived, against the clause below] Basis RFC 9110 clause 8.4.1.2: "The "deflate" coding is a "zlib" data format [RFC1950] containing a "deflate" compressed data stream [RFC1951] that uses a combination of the Lempel-Ziv (LZ77) compression algorithm and Huffman coding." So "deflate" means the zlib wrapper, windowBits 15, and gzip is 15 with 16 added. The decode side already implemented this correctly; only the encode side did not. Raised by Auditing the compression path while fixing the loop in the same function. Change Compute windowBits from the declared encoding and use it for deflateInit2, making the two halves symmetric. General FLUTE in practice: the MBMS Download Profile permits no Content-Encoding other than gzip, and a session on that profile now refuses anything else outright, so the deflate branch is only ever taken by a non-3GPP caller. It is still wrong to write one framing and read another. Verification T0. The change is a one-expression edit whose correctness rests on the quoted clause and on matching the decode side, which was already right, and the suite still passes at 44 cases. No unit test, for the reason recorded two commits earlier: encode() runs inside the File constructor and reaching it with a usable FEC OTI requires Transmitter::FileDescription, whose merge_fec_oti() is protected, so a test cannot configure one without a Transmitter with sockets and an io_context. A round-trip test over both framings is the right test and is blocked on that gap, which is recorded rather than worked around. Not in this change No change to decode(), which was already correct, and no change to which encodings are accepted.
Problem Any content-encoded object larger than the decompression buffer failed to decode. File::decode() called inflate() with Z_FINISH, which asserts to zlib that the output buffer can take the whole remaining stream; with a fixed 16384-byte buffer that is untrue for any larger object, so inflate returned an error and the object was discarded. Observed: a 100000-byte gzip object logs a decompression error and never completes. Code-derived, no spec claim. Raised by live testing of the compression path Change Both inflate() calls now pass Z_NO_FLUSH, which is the mode for feeding a stream through a buffer smaller than its output, and the loop already handles the partial-output case. Also substitutes a fixed string when zlib supplies no message and releases the stream before throwing, so a decode failure reports rather than formatting a null pointer. Verification T2: a 100000-byte gzip object reaches Z_STREAM_END in six iterations with all 100000 bytes recovered, where before it failed on the first. Not in this change The Content-Length substitution in the FDT parser, which is a separate defect and the next commit.
…file allows it
Problem
Under the MBMS Download Profile a content-encoded object could not be decoded
by any receiver. Observed live: a gzip object transmitted and received on
loopback ends "Decompressed length does not match expected Content-Length"
and the object is discarded. The FDT parser on this base already declines to
substitute Content-Length for an absent Transfer-Length once an encoding is
applied, which is correct, but nothing then supplies the real length and three
things stand in the way:
- reception was started for an object whose length the FDT never stated, so
the object was partitioned to a length that was simply not known
(src/Receiver.cpp);
- adopt_fdt_metadata() took neither Content-Encoding nor Content-Length from
the FDT entry, so an object whose reception bootstrapped from EXT_FTI was
delivered still compressed and then length-checked against the encoded size
(include/File.h);
- and the sender put EXT_FTI on the FDT packet only, gating both extensions
behind "if (toi == 0)", so the one permitted route for the transfer length
carried nothing (src/AlcPacket.cpp).
[observed, code-derived]
Basis
The profile obliges a receiver to handle these objects.
TS 26.346 V18.2.0 clause L.4.2, second list: "Content-Encoding set to 'gzip'"
is the single item of the list introduced as "may be carried in the FDT sent
by the FLUTE sender, and shall be supported by the FLUTE receiver".
It also forbids the attribute that would state the length.
TS 26.346 V18.2.0 clause L.4.4, fourth list: "The following attributes shall
not be carried in the FDT sent by the FLUTE sender:", Transfer-Length first.
And Content-Length may not stand in for it here.
RFC 3926 clause 3.4.2: "If the file is not content encoded before transport
(and thus the "Content-Encoding" attribute is not used) then the transfer
length is the length of the original file, and in this case the
"Content-Length" is also the transfer length."
That leaves the object's own EXT_FTI, which every receiver must support, and
reaching for it departs from a "should" rather than a "shall not".
TS 26.346 V18.2.0 clause L.4.7: "FEC Object Transmission Information in FLUTE
packets which carry symbols of content files should be conveyed by the
FEC-OTI parameters in the FDT". Every FEC-OTI parameter the profile permits is
still in the FDT; only the transfer length travels in band.
Raised by
a maintainer's direction that the 3GPP receive path be specification
compliant, which turned a recorded stop-and-ask into a decision
Change
The receiver defers starting an object whose length the FDT does not state,
takes it from the object's EXT_FTI when the first packet arrives, and adopts
the waiting FDT entry immediately so the object still lands at its
Content-Location and is decoded per its Content-Encoding.
adopt_fdt_metadata() now carries Content-Encoding and Content-Length across
while still leaving fec_oti to the in-band value. The sender emits EXT_FTI on
a content packet exactly when the FDT cannot state the length, which under
this profile is a content-encoded object and nothing else. Also corrects the
comment beside the sender-side prohibition, which claimed nothing is lost by
withholding Transfer-Length; that holds only for an unencoded object.
Verification
T1: EncodedObjectTransferLengthTest, 3 new cases covering the substitution
rule in both directions and an explicit Transfer-Length overriding both;
whole suite 64 cases passing.
T2: live over real multicast. A gzip object is received byte-identical,
sha256 a4366ca61727259f94ba..., the same digest as that object sent
uncompressed. The identical run discarded it before this change. An
uncompressed transfer still arrives byte-identical.
Not in this change
Emitting Transfer-Length in the FDT, which clause L.4.4 forbids.
The FDT parser's substitution rule and the decompression path itself, both of
which are already on this branch's base.
Problem The profile permits an object to be content encoded but provides no carrier for the resulting transfer length, and this sender used one of the two the profile forbids. It attached EXT_FTI to the content packets of an encoded object, which clause 7.2.8 prohibits, having correctly avoided the FDT's Transfer-Length attribute, which clause L.4.4 prohibits. [source-derived] Basis TS 26.346 V18.2.0 clause 7.2.8: "-FLUTE packets carrying symbols of files (not FDT Instances) shall not include an EXT_FTI." TS 26.346 V18.2.0 clause L.4.4: "The following attributes shall not be carried in the FDT sent by the FLUTE sender:" listing Transfer-Length first. RFC 3926 clause 3.4.2, on why Content-Length cannot stand in: "then the transfer length is generally different than the length of the original file, and in this case the attribute "Transfer-Length" MAY be used to carry the transfer length." With both carriers closed and Content-Length not applicable, an encoded object under this profile cannot state its length by any route, yet clause L.4.2 permits the encoding. That contradiction is raised as 5G-MAG/Standards#212 and is not resolved here. Declining to encode is conformant meanwhile, because clause L.4.2 leaves it to the sender: "The following FDT attribute, defined at both the FDT-Instance and File levels, may be carried in the FDT sent by the FLUTE sender" Raised by reading the authority during this work Change Transmitter owns the choice, so it is made there. Under Profile::Mbms3gpp, send() refuses a file whose FileDescription asked for compression, naming the reason and the Standards issue, rather than silently sending it uncompressed and leaving the caller to believe otherwise. EXT_FTI is no longer attached to any content packet under that profile. Outside the profile both the encoding and the extension are unchanged, and RFC 3926 permits Transfer-Length there. Receiving is untouched. Clause L.4.2 requires a receiver to support gzip and this library does, so a content-encoded object from a sender that reads the profile differently still decodes. Verification T1: 60 cases pass, 3 new. ProfileContentEncodingTest covers refusal under the profile, acceptance outside it, and that an unencoded object is unaffected. No test previously reached this path, which is why the change was silent in the suite before they were added. Not in this change No resolution of the contradiction, which is 3GPP's to make. If it is resolved in favour of EXT_FTI on content packets, the gate here is one condition to relax.
1d6b22b to
a1a4798
Compare
This pull request: keep it in draft, and it needs a decisionStatus: draft, and it should stay that way for now. 9 commits, 11 files, +552/-36. 77 cases pass, and everything in it except one commit is ready. What blocks it: 5G-MAG/Standards#212, still open with no replies. TS 26.346 permits gzip in the MBMS Download Profile (annex L.4.2) but forbids both of the carriers that would let a receiver learn the transfer length of a gzipped object: annex L.4.4 forbids What to do, two options
I would take option 2 if Standards#212 looks like taking more than a couple of weeks, and option 1 otherwise. It is your call, and either way nothing here should merge while the blocked commit is in it. One thing to re-read if you reviewed this before. Rebasing onto #62 moved two guards #62 adds into How this set of pull requests came to look like this
Where the nine closed pull requests went
One correction, @rjb1000. #93 is listed above as "already merged in PR #60". #60 merged the FDT growth fix on 17 August and changed no test file; Every open issue, and which pull request closes it
Four open issues are deliberately closed by nothing:
The stack, and the order to merge it inGitHub retargets each child to One trap for whoever merges. The heads live in two repositories: #61, #68 and #98 in |
|
Thank you @rjb1000, both points settled. Rebase done. One thing to re-read: two guards from #62 now sit inside RaptorQ: you were right, it is not here. That is #64. Leave in draft. Blocked on 5G-MAG/Standards#212: TS 26.346 permits gzip but forbids both carriers that would tell a receiver a gzipped object's transfer length. A specification defect, not one to resolve by picking an interpretation. Decision for you. The block is one commit, closing #80. #66, #77, #78 and #79 are ready and independent, so if Standards#212 looks slow, splitting that commit out lets four issues close now. |
Closes #66, closes #77, closes #78, closes #79, closes #80.
Dependencies and issues
Depends on: #62, and is based on its branch, so #62 merges first.
Paired with: #61, #64, #65 and #98, independent of this one, either order.
Closes on merge: #66, #77, #78, #79, #80.
Note: #80 needs #79's parser change, and both are in this pull request, so they land together.
What is in this pull request
Eight commits of its own, out of the 33 shown against
development. The other 25 are #62's.Content-Lengthsubstituting for a missingTransfer-Lengthonly where no content encoding was declared, since RFC 3926 clause 3.4.2 authorises it for that case and no otherchar*that aborts the process; decompression usesZ_NO_FLUSHso an object larger than the 16 KB buffer decodes; framing follows the declared encoding instead of being fixed at gzipOn the compression commits
PR #69, merged 21 August, corrected which zlib function the compression loop calls. That was a one-line
change and fixes none of the three here, so these are additional to it, not a duplicate of it.
One behaviour change from the rebase onto #62
This branch's first commit factors the receive handler into
process_alc_datagram(), shared by the plainand tunnelled receive loops, and each loop arms its own socket after calling it. Two guards that #62 adds
inside that function returned by way of
arm_receive(), which the shared contract makes wrong: it lefttwo outstanding reads on the plain socket, and armed the plain socket from a datagram that arrived through
the tunnel. Those calls are gone and a comment at the function records why discards there return instead.
Reviewers who looked at the plain-path version of these guards on #62 should read them again here; the
function they sit in is not the one they were written for.
Verification
T1: 77 cases passing. T2: plain and gzip transfers received byte-identical over real multicast, the gzip
object at sha256
a4366ca61727259f94ba…, the same digest as that object sent uncompressed. Before the#80 work the identical run received and then discarded it.
Merge order
After #62. Independent of the FEC branches.